feat: test pg_upgrade compatibility with older extension versions #1897
Conversation
samrose
left a comment
samrose
left a comment
There was a problem hiding this comment.
Looks great! Will just need to test before we merge it
jfroche
force-pushed
the
feat/test-pg-upgrade-older-version
branch
from
8acfa60 to
f59c791
Compare
yvan-sraka
force-pushed
the
feat/test-pg-upgrade-older-version
branch
from
f59c791 to
779090b
Compare
yvan-sraka
force-pushed
the
feat/test-pg-upgrade-older-version
branch
2 times, most recently
from
24d66bb to
164f401
Compare
yvan-sraka
force-pushed
the
feat/test-pg-upgrade-older-version
branch
from
164f401 to
450b690
Compare
This comment has been minimized.
This comment has been minimized.
yvan-sraka
force-pushed
the
feat/test-pg-upgrade-older-version
branch
from
f6f9d9b to
9beb48a
Compare
yvan-sraka
force-pushed
the
feat/test-pg-upgrade-older-version
branch
from
9beb48a to
218357a
Compare
jfroche
force-pushed
the
custom-github-runners
branch
from
c1c4fd7 to
9b659e4
Compare
yvan-sraka
force-pushed
the
feat/test-pg-upgrade-older-version
branch
2 times, most recently
from
971248f to
84cfa8d
Compare
yvan-sraka
force-pushed
the
custom-github-runners
branch
from
efd8f49 to
a5fc472
Compare
yvan-sraka
force-pushed
the
feat/test-pg-upgrade-older-version
branch
2 times, most recently
from
39db71b to
29a4d56
Compare
yvan-sraka
force-pushed
the
custom-github-runners
branch
2 times, most recently
from
03dc754 to
c159bb6
Compare
jfroche
force-pushed
the
custom-github-runners
branch
3 times, most recently
from
df5545b to
b9294ee
Compare
jfroche
force-pushed
the
custom-github-runners
branch
21 times, most recently
from
182d513 to
1246b61
Compare
yvan-sraka
force-pushed
the
feat/test-pg-upgrade-older-version
branch
from
29a4d56 to
d36e133
Compare
Add test to verify that all extension versions from PostgreSQL 15 can successfully upgrade to PostgreSQL 17 using pg_upgrade. The test now validates: - Each PG 15 extension version can be upgraded to PG 17 - Extension update scripts are properly generated during upgrade - Version handling works correctly with and without update scripts - Final extension versions match expected values after upgrade
…ade compatibility When upgrading PostgreSQL versions, pg_upgrade needs access to old extension versions sql to migrate from. This adds unsupported pg_graphql versions (those not compilable with current PostgreSQL) as SQL-only packages, ensuring migration paths exist when upgrading from older PostgreSQL versions.
…pgrade compatibility When upgrading PostgreSQL versions, pg_upgrade needs access to old extension versions sql to migrate from. This adds unsupported pg_graphql versions (those not compilable with current PostgreSQL) as SQL-only packages, ensuring migration paths exist when upgrading from older PostgreSQL versions.
When upgrading from PostgreSQL 15 to 17, pg_stat_monitor version 1.0 (PG 15-only) cannot be migrated as it uses .sql.in template files that reference MODULE_PATHNAME without proper processing for the target version. This marks version 1.0 as not pg_upgrade compatible and filters it from the version test list, allowing the test to use version 2.1 (which supports both PG 15 and 17) for pg_upgrade validation instead. Version 1.0 remains available for PG 15 installations. Version 2.1 has different schemas on PostgreSQL 15 vs 17 despite sharing the same version number. On PG 15 it uses the older schema with blk_read_time and blk_write_time columns, while on PG 17 it uses a newer schema with shared_blk_read_time, shared_blk_write_time, local_blk_read_time, local_blk_write_time and additional JIT statistics columns. During pg_upgrade from PG 15 to 17, the extension version remains 2.1 without schema migration since no update script is generated. Fresh installations on PG 17 receive the new schema while pg_upgrade retains the old schema, creating a test conflict as both scenarios share the same expected output file. A custom test implementation skips pg_regress validation after pg_upgrade when no update script is generated, since the schema mismatch is expected behavior. This maintains full test coverage for fresh installations through the regular psql_17 check while validating extension version compatibility for pg_upgrade scenarios.
jfroche
force-pushed
the
feat/test-pg-upgrade-older-version
branch
from
d36e133 to
423dd3d
Compare
WalkthroughThis PR introduces PostgreSQL upgrade path testing and unsupported version handling across multiple extension build configurations. Changes include extracting version variables (pgVersion, pgUpgradeCompatible), adding SQL migration generation logic, refactoring test infrastructure to support multi-version upgrade workflows, and introducing new test configurations for pg_stat_monitor while restructuring existing test harnesses to use external Python modules. Changes
Sequence Diagram(s)sequenceDiagram
participant Tester
participant PostgreSQL 15
participant FileSystem as File System
participant Extension as Extension System
participant PostgreSQL 17
Tester->>PostgreSQL 15: Stop PostgreSQL
Tester->>FileSystem: Reset data directory
Tester->>PostgreSQL 15: Switch to PG 15 config
PostgreSQL 15->>PostgreSQL 15: Start with v15
Tester->>Extension: Drop extension (CASCADE)
Tester->>Extension: Install older extension version
Tester->>PostgreSQL 17: Switch to PG 17 config
PostgreSQL 15->>PostgreSQL 17: Perform pg_upgrade
Tester->>FileSystem: Check for update_extensions.sql
alt Update script exists
Tester->>Extension: Execute update script
Extension->>Extension: Upgrade extension schema
Tester->>Tester: Assert latest v17 version
else No update script
Tester->>Tester: Assert v15 version persists
end
Tester->>PostgreSQL 17: Run pg_regress validation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@nix/ext/pg_stat_monitor.nix`:
- Around line 20-29: latestVersion is computed from allVersionsList but packages
are built from pgUpgradeCompatibleVersions, causing a mismatch if the newest
supported version is not pgUpgradeCompatible; change the latestVersion
definition to derive from the pgUpgradeCompatible set (e.g. use lib.last on the
attrNames of pgUpgradeCompatibleVersions or on a sorted list of
pgUpgradeCompatible version keys) so latestVersion and packages come from the
same pg_upgrade-compatible collection (update the latestVersion variable to
reference pgUpgradeCompatibleVersions instead of allVersionsList).
In `@nix/ext/tests/pg_stat_monitor.nix`:
- Around line 190-191: The PostgresExtensionTest is being instantiated without
the computed lib_name, causing the constructor to default to extension_name;
update the PostgresExtensionTest(...) call that currently uses (server,
extension_name, versions, sql_test_directory, support_upgrade, ext_schema) so it
includes the computed lib_name argument (the variable lib_name) in the parameter
list passed to PostgresExtensionTest, ensuring subsequent uses (e.g.,
test.create_schema() and later references to lib_name) target the correct
library name.
🧹 Nitpick comments (2)
nix/ext/pg_jsonschema/default.nix (1)
96-96: Consider removing or making debug output explicit.The
find $outcommand appears to be debugging output that lists build artifacts. If intentional for troubleshooting, consider adding a comment explaining its purpose or wrapping it in anechostatement. If unintentional, it can be removed.🔧 Suggested change if keeping for debugging
postInstall = '' - find $out + # Debug: list build artifacts + echo "Build artifacts:" && find $out mv $out/lib/${pname}${postgresql.dlSuffix} $out/lib/${pname}-${version}${postgresql.dlSuffix}Or remove if not needed:
postInstall = '' - find $out mv $out/lib/${pname}${postgresql.dlSuffix} $out/lib/${pname}-${version}${postgresql.dlSuffix}nix/ext/tests/pgsodium.nix (1)
197-223: Ensure pg_upgrade completes before checking update script.
switch-to-configurationcan return before services are fully ready; the immediate check forupdate_extensions.sqlcan race and fliphas_update_scripttono. Consider waiting forpostgresql.service(orpostgresql-migrate.service) after each switch.🔧 Suggested wait to reduce flakiness
server.succeed( f"{pg15_configuration}/bin/switch-to-configuration test >&2" ) + server.wait_for_unit("postgresql.service") test.drop_extension() test.install_extension(version) server.succeed( f"{pg17_configuration}/bin/switch-to-configuration test >&2" ) + server.wait_for_unit("postgresql.service") has_update_script = server.succeed( "test -f /var/lib/postgresql/update_extensions.sql && echo 'yes' || echo 'no'" ).strip() == "yes"
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
nix/ext/pg_graphql/default.nixnix/ext/pg_jsonschema/default.nixnix/ext/pg_stat_monitor.nixnix/ext/tests/default.nixnix/ext/tests/lib.pynix/ext/tests/pg_stat_monitor.nixnix/ext/tests/pgmq.nixnix/ext/tests/pgroonga.nixnix/ext/tests/pgsodium.nixnix/ext/tests/vault.nixnix/ext/versions.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: nix-eval / eval
🔇 Additional comments (26)
nix/ext/tests/lib.py (1)
57-58: LGTM - CASCADE ensures clean teardown.Adding
CASCADEto theDROP EXTENSIONcommand is appropriate for test cleanup, ensuring dependent objects (views, functions, etc.) are properly removed between test iterations. This aligns with the multi-version upgrade testing scenarios introduced in this PR.nix/ext/pg_graphql/default.nix (3)
122-124: LGTM - Centralizing pgVersion improves maintainability.Extracting
pgVersionas a variable and using it consistently throughout the file is a good refactoring that aligns with the same pattern inpg_jsonschema/default.nix.
132-162: LGTM - SQL-only packages enable pg_upgrade for unsupported versions.The
buildUnsupportedfunction correctly creates minimal SQL-only extension packages that pg_upgrade needs to reference old extension versions without requiring them to compile against the new PostgreSQL version. The approach of concatenating SQL files with semicolon separators is appropriate for this migration-only use case.
163-179: LGTM - Conditional inclusion of SQL-only packages for pg_upgrade.The logic correctly:
- Filters versions incompatible with the current
pgVersion- Skips generating SQL-only packages for PG15 (no older versions to upgrade from)
- Includes SQL-only packages in the build environment for PG17+ to support pg_upgrade
nix/ext/tests/pgroonga.nix (2)
141-143: LGTM - Configuration path variables.The explicit system and configuration path variables improve readability and provide clear references for switching between PG15 and PG17 configurations during upgrade testing.
196-221: LGTM - Comprehensive pg_upgrade test for all PG15 extension versions.This test thoroughly validates the upgrade path by:
- Testing each PG15 extension version individually
- Handling both scenarios: with update scripts (upgrade to latest) and without (version preserved)
- Running pg_regress validation after each upgrade
The clean state reset (stopping postgres, removing data) between iterations ensures isolated test runs.
nix/ext/pg_jsonschema/default.nix (2)
78-80: LGTM - Consistent pgVersion usage in preCheck.The
pgVersionvariable is used consistently for rsync paths and cargo pgrx initialization, matching the pattern established inpg_graphql/default.nix.
135-136: LGTM - Unsupported version handling for pg_upgrade.The approach correctly:
- Identifies PG15-only versions that won't compile against PG17
- Only generates placeholders when building for PG17
- Copies SQL from the earliest supported version as a migration placeholder
Note: This differs from
pg_graphqlwhich concatenates source SQL files. The simpler copy approach here works because pg_jsonschema's SQL structure allows it.Also applies to: 153-155
nix/ext/versions.json (1)
778-779: LGTM - Marking pg_stat_monitor 1.0 as pg_upgrade incompatible.The
pgUpgradeCompatible: falseflag correctly excludes this PG15-only version from pg_upgrade testing. The field is properly consumed innix/ext/pg_stat_monitor.nix(lines 21, 25-29) to filter out incompatible versions from both the version list and build packages. Version 2.1, which supports both PG15 and PG17, will be used for upgrade validation instead.nix/ext/tests/pgmq.nix (2)
122-125: LGTM: system-derived pg15/pg17 configuration paths.This keeps configuration switching consistent with other test files.
182-205: LGTM: pg_upgrade coverage across older PG15 extension versions.The loop validates upgrade behavior and update-script handling in a clear, repeatable way.
nix/ext/tests/vault.nix (2)
149-151: LGTM: consistent pg15/pg17 configuration derivation.
206-233: LGTM: pg_upgrade loop validates older-version upgrades and regression coverage.nix/ext/tests/default.nix (3)
153-155: LGTM: system-based pg15/pg17 configuration paths.
212-237: LGTM: pg_upgrade loop for older extension versions.
261-264: LGTM: extension list update (pg_partman, postgis).nix/ext/tests/pg_stat_monitor.nix (2)
30-119: LGTM: dedicated pg_stat_monitor test harness with pg15/pg17 specialisation.
230-264: LGTM: conditional pg_regress skip when no update script is generated.Clear rationale and targeted handling for schema divergence after pg_upgrade.
nix/ext/tests/pgsodium.nix (8)
50-51: LGTM: explicit psql_15/psql_17 bindings.
Keeps reuse consistent across service and migration configuration.
71-85: Verify peer auth mapping matches how tests connect.
Withlocal all postgres ... map=postgresand nopostgres root postgresentry, anypsql -U postgresexecuted as root will fail. Please confirm the harness usessudo -u postgres(or similar), or add a mapping if root needs to connect aspostgres.Optional fallback if root must connect as postgres
identMap = '' root root supabase_admin + postgres root postgres postgres postgres postgres '';
94-94: LGTM: pg17 specialization uses psql_17.
108-109: LGTM: explicit old/new PostgreSQL packages for pg_upgrade.
136-151: LGTM: harness wiring with explicit config variables.
158-175: LGTM: PG15 upgrade/pg_regress coverage.
179-179: LGTM: switch uses pg17_configuration.
182-195: LGTM: PG17 regression and last-version checks.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| allVersionsList = lib.naturalSort (lib.attrNames supportedVersions); | ||
| versions = builtins.filter (v: (allVersions.${v}.pgUpgradeCompatible or true)) allVersionsList; | ||
| latestVersion = lib.last allVersionsList; | ||
| numberOfVersions = builtins.length versions; | ||
| # Filter to only build pg_upgrade compatible versions | ||
| pgUpgradeCompatibleVersions = lib.filterAttrs ( | ||
| name: _: allVersions.${name}.pgUpgradeCompatible or true | ||
| ) supportedVersions; | ||
| packages = builtins.attrValues ( | ||
| lib.mapAttrs (name: value: build name value.hash value.revision) supportedVersions | ||
| lib.mapAttrs (name: value: build name value.hash value.revision) pgUpgradeCompatibleVersions |
There was a problem hiding this comment.
Derive latestVersion from the pg_upgrade-compatible set.
latestVersion currently comes from allVersionsList (supported), while packages are built from pgUpgradeCompatibleVersions. If the newest supported version is marked pgUpgradeCompatible = false, no derivation will generate the default control file/symlink, and the post-build file-count check will fail. Consider sourcing latestVersion from the compatible list to keep build outputs consistent.
🛠️ Proposed fix
- latestVersion = lib.last allVersionsList;
+ latestVersion = lib.last versions;🤖 Prompt for AI Agents
In `@nix/ext/pg_stat_monitor.nix` around lines 20 - 29, latestVersion is computed
from allVersionsList but packages are built from pgUpgradeCompatibleVersions,
causing a mismatch if the newest supported version is not pgUpgradeCompatible;
change the latestVersion definition to derive from the pgUpgradeCompatible set
(e.g. use lib.last on the attrNames of pgUpgradeCompatibleVersions or on a
sorted list of pgUpgradeCompatible version keys) so latestVersion and packages
come from the same pg_upgrade-compatible collection (update the latestVersion
variable to reference pgUpgradeCompatibleVersions instead of allVersionsList).
| test = PostgresExtensionTest(server, extension_name, versions, sql_test_directory, support_upgrade, ext_schema) | ||
| test.create_schema() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find and examine the PostgresExtensionTest class definition
echo "=== Searching for PostgresExtensionTest class ==="
fd -a '^lib.py$' nix/ext/tests | while read -r f; do
echo "### Found: $f"
echo "=== Class definition and __init__ ==="
rg -n -A 20 "class PostgresExtensionTest" "$f"
done
# Also search broadly in case it's in a different location
echo ""
echo "=== Broader search for PostgresExtensionTest ==="
rg -l "class PostgresExtensionTest"Repository: supabase/postgres
Length of output: 1163
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the pg_stat_monitor.nix file to see if lib_name is computed
echo "=== Checking pg_stat_monitor.nix for lib_name ==="
fd -a '^pg_stat_monitor.nix$' nix/ext/tests && cat nix/ext/tests/pg_stat_monitor.nix | grep -n -i "lib_name" || echo "No lib_name found"
# Also check the context around lines 190-191
echo ""
echo "=== Context around lines 180-200 in pg_stat_monitor.nix ==="
sed -n '180,200p' nix/ext/tests/pg_stat_monitor.nixRepository: supabase/postgres
Length of output: 1288
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for other test files in the nix/ext/tests directory and see how they call PostgresExtensionTest
echo "=== Searching for PostgresExtensionTest calls in other test files ==="
fd -a '\.nix$' nix/ext/tests | while read -r f; do
if rg -q "PostgresExtensionTest" "$f"; then
echo "### File: $f"
rg -n -B 2 -A 2 "PostgresExtensionTest" "$f" | head -30
fi
doneRepository: supabase/postgres
Length of output: 3860
Pass the computed lib_name to PostgresExtensionTest.
The file computes lib_name on line 180 but doesn't pass it when instantiating PostgresExtensionTest on lines 190–191. Without this parameter, the constructor defaults to using extension_name instead. Since the computed lib_name explicitly differs from the extension name (based on the fallback logic on line 180) and is used later in the test on line 205, it should be passed to ensure the test targets the correct library.
Proposed fix
- test = PostgresExtensionTest(server, extension_name, versions, sql_test_directory, support_upgrade, ext_schema)
+ test = PostgresExtensionTest(server, extension_name, versions, sql_test_directory, support_upgrade, ext_schema, lib_name)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test = PostgresExtensionTest(server, extension_name, versions, sql_test_directory, support_upgrade, ext_schema) | |
| test.create_schema() | |
| test = PostgresExtensionTest(server, extension_name, versions, sql_test_directory, support_upgrade, ext_schema, lib_name) | |
| test.create_schema() |
🤖 Prompt for AI Agents
In `@nix/ext/tests/pg_stat_monitor.nix` around lines 190 - 191, The
PostgresExtensionTest is being instantiated without the computed lib_name,
causing the constructor to default to extension_name; update the
PostgresExtensionTest(...) call that currently uses (server, extension_name,
versions, sql_test_directory, support_upgrade, ext_schema) so it includes the
computed lib_name argument (the variable lib_name) in the parameter list passed
to PostgresExtensionTest, ensuring subsequent uses (e.g., test.create_schema()
and later references to lib_name) target the correct library name.
Add test to verify that all extension versions from PostgreSQL 15 can successfully upgrade to PostgreSQL 17 using pg_upgrade.
The test now validates:
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.